Default to signed for all int columns, opt-in for unsigned#956
Default to signed for all int columns, opt-in for unsigned#956dereuromark merged 13 commits into5.xfrom
Conversation
| * Should the column be signed? | ||
| * | ||
| * @return bool | ||
| * @deprecated 5.0 Use isUnsigned() instead. |
There was a problem hiding this comment.
We should deprecate this on the Cake ORM maybe?
We dont need isUnsigned() and isSigned() at the same time, do we?
There was a problem hiding this comment.
We should deprecate this on the Cake ORM maybe?
The ORM API is part of the additions in 5.3. If we deprecate that it requires changes to cakephp/database and the orm. I thought we could align on what we already have in the core.
There was a problem hiding this comment.
What do you propose we do?
Since it we shouldnt have both positive and negative form at the same time here in the new major.
Or in other words: Wouldn't it be cleaner to only have one side, and not the other on top?
There was a problem hiding this comment.
Or in other words: Wouldn't it be cleaner to only have one side, and not the other on top?
Yes of course, one way would be preferable. However migrations and cake started in opposite directions, and in order for us to provide backwards compatibility we have to support both for a while.
There was a problem hiding this comment.
Thus my proposal to deprecate but keep around.
otherwise feel free to modify as needed so we can merge and move forward
|
I adjusted it as per my comment. A few things are confusing: If we move forward with signed by default, and unsigned as opt-in, it could make sense to use *Unsigned() here as it would be default false, and then opt-in true. So a few discussions we still have to make until we can move this PR out of draft. |
dereuromark
changed the title
|
Ping @markstory @LordSimal @ADmad |
|
Well it may be inconsistent, but by definition an integer can still contain negative values. Thats why a non PK column will be signed to also allow negative values. A primary key on the other hand is by design auto incremented and will always start at 1 and therefore will never reach negative values. Making that unsigned is obvious. We already have So instead of trying to force sync up the integer column type and primary key column type I'd rather try to detect, if the given integer column is actually a foreign key column and therefore recommend using a different migration method ( to add it |
|
But how do you properly detect this? foreign keys can not always be Please all, also address the open questions here ("confusions"), that are also important as to the "clean API". |
|
Guide added |
dereuromark
force-pushed
the
5.x-unsigned
branch
from
17b6a03 to
6c61825
Compare
|
Are we OK to move forward? |
| $this->assertTrue($column->isSigned()); | ||
| } | ||
|
|
||
| #[RunInSeparateProcess] |
There was a problem hiding this comment.
Why do these tests need to run in a separate process? They look safe to run in the current process.
There was a problem hiding this comment.
The #[RunInSeparateProcess] is used here because these tests modify Configure settings, and without process isolation, those settings would persist and affect other tests.
However, there's a simpler solution: use setUp()/tearDown() to save and restore the Configure values, which avoids the overhead of spawning separate processes.
There was a problem hiding this comment.
But the default teardown in TestCase already handles resetting the state of configure.
Use tearDown() to restore Configure defaults instead of running tests in separate processes. This improves test performance.
The adapter tests (MysqlAdapterTest, etc.) drop and recreate the database in their setUp method, which destroys the schema tables created by SchemaLoader. MigrationHelperTest then fails because the users and special_tags tables no longer exist. Use #[RunTestsInSeparateProcesses] to ensure MigrationHelperTest runs in isolation with its own fresh schema.
|
Folks, can we please move forward here? We need to start finishing the 5.x topic, so we can release this soon after Cake53 stable |
4 participants
Addresses Issue: cakephp/phinx#2267
Auto-generated primary keys default to unsigned, but manually added integer columns default to signed. This creates FK mismatches and requires explicit
'signed' => falsedeclarations.Reverts the primary key part of https://github.com/cakephp/phinx/pull/2159/files which is actually harmful as default.
This needs to be set together with all foreign keys or it will create invalid definitions.
So we can either
This PR aims to restore sane defaults, and people can opt-in for "optimization".